ElectronAnalyser no longer depends directly on sequence file path to work with BlueAPI#1923
ElectronAnalyser no longer depends directly on sequence file path to work with BlueAPI#1923oliwenmandiamond wants to merge 7 commits intomainfrom
Conversation
…work with BlueAPI
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1923 +/- ##
==========================================
- Coverage 99.05% 99.05% -0.01%
==========================================
Files 312 312
Lines 11754 11735 -19
==========================================
- Hits 11643 11624 -19
Misses 111 111 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f807817 to
7d4d359
Compare
| ): | ||
| self._sequence_class = sequence_class | ||
| # Save on device so connect works and names it as child | ||
| self.driver = controller.driver |
There was a problem hiding this comment.
this is a driver that is created and passed in a child class, here in parent class it is taken out from the controller and saved as a class object - wouldn't it be clearer to make it in a child class directly when driver is created?
There was a problem hiding this comment.
The class is then in charge of making sure the driver and detector connect and parent/name relationship is correct rather than the API. It's much better to make sure the API does it as all classes will need to do this, otherwise we copy and paste this line x number times for number of implementations which is unnecessary.
There was a problem hiding this comment.
you probably won't need to copy paste anything just add "self.driver" instead of making internal "driver" in a child class
There was a problem hiding this comment.
The issue with this is that if we type anything as using the GenericElectronAnalyser for tests or plans or other devices i.e it doesn't care about if it Specs, Mbs, VGScienta etc. then it loses access to the driver because it lives now on the implementation and not on the base. We need it to be done here.
There was a problem hiding this comment.
it looks like your use case for this is only relevant for tests, you probably could in analyser-related plan specify a parameter as GenericElectronAnalyser in a plan but you anyway want to operate using verbs so Movable, Readable or StandardDetector class (when it's there) would suffice? Isn't it the whole point of this change to get rid of generic analyser, am I missing smth?
There was a problem hiding this comment.
or are you planning to call bps.move(analyser.driver) in a plan?
There was a problem hiding this comment.
If the Bluesky change goes through with allowing devices have multiple configuration, then we can remove method create_region_detector_listand class GenericElectronAnalyserRegionDetector and GenericBaseElectronAnalyserDetector and just have only GenericElectronAnalyserDetector. We have to live with this complexity until this is done. This change is focusing on working with what what we got and letting it work with BlueAPI by removing file path dependency. The generic classes currently are only used in tests yes. They are a very convenient way to keep full typing for classes. Otherwise to have full typing I would have to do something like below
ElectronAnalyserDetector[AbastractRegion, AbstractDriverIO[AbstractRegion]]
and that isn't including the PsuMode, PassEnergy and LensMode types.
I think the reason they aren't used in the plans themselves is because they are invariant so has a couple of issues, but again this isn't related to this change.
Plans can still have full typing of the device and would need it to access child devices e.g
E.g
def my_plan(my_device: Movable):
yield from mv(my_device.child_device1, 2) #invalid type checking
yield from mv(my_device.child_device2, 5) #invalid type checkingIf you give the correct type for the plan static type checking won't complain.
If you dislike the generics classes because they are only used in tests and not plans at the moment we can address this in a new issue/PR.
There was a problem hiding this comment.
I rather dislike that inverse logic of having driver in base class via controller in child class that actually creates that driver -> which seems to be justified only for having GenericElectronAnalyser device having access to driver which is used only in tests and plan tests.
| @@ -201,7 +187,7 @@ def create_region_detector_list( | |||
|
|
|||
|
|
|||
| GenericElectronAnalyserDetector = ElectronAnalyserDetector[ | |||
There was a problem hiding this comment.
do you need this class here? it seems to be used only in tests
There was a problem hiding this comment.
Yeah, this will be used in plan repo too.
There was a problem hiding this comment.
which plan repo? sm-bluesky?
There was a problem hiding this comment.
Fixes #1924
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}